Add wait-change API endpoint and client function #63
Conversation
client/changes.go
Outdated
| // If non-zero, wait at most this long before returning the current change | ||
| // data. The timeout elapsing is not considered an error, so if a timeout | ||
| // is specified, the caller should check Change.Ready to determine whether | ||
| // the change is actually finished. |
There was a problem hiding this comment.
Timeout certainly sounds like an error, unless the operation did finish and there was a race condition. But otherwise, the intent of the operation of waiting for it to be finished did not succeed in intent, so an error seems appropriate.
There was a problem hiding this comment.
Yeah, Harry and I went back and forth on that and ended up that it shouldn't be an error, primarily because when Pebble returns an error it can't also return other data (the Change in this case), and I thought it might be useful to have the current state of the change at the time of the timeout.
However, on reflection it makes for a less error-prone API if it's just an error, and in the success case the change is always ready. If the caller really wants the current state of a change in the case of a timeout/error, they can always make an additional call to client.Change() to get the current status. However, it's unlikely you'll want to.
In any case, I've used HTTP status code 504 Gateway Timeout for this (along with a reasonable error message). It's not quite a perfect semantic match, but it seems close enough.
| query.Set("timeout", opts.Timeout.String()) | ||
| } | ||
|
|
||
| _, err := client.doSync("GET", "/v1/changes/"+id+"/wait", query, nil, nil, &chgd) |
There was a problem hiding this comment.
What's the timeout behavior of the underlying http library, and do we have to account for that and implement retries here?
There was a problem hiding this comment.
Good question. I had observed it to be a very long timeout / no timeout in my manual testing, but just confirmed that now in the code -- we always create the http.Client with no Timeout specified, so no timeout applies here. Arguably that's bad practice, but we can follow up in a subsequent PR if we want to change that.
|
@niemeyer I've addressed your concerns -- mainly making the timeout an error and tweaking the doc comments. I'm going to go ahead and merge; we can always tweak later if necessary. |
Corresponds to the new wait-change API endpoint in canonical/pebble#63. This uses the new API if it's present, otherwise falls back to the existing polling-every-100ms method.
More specifically, these PRs: * canonical/pebble#59 Add GPLv3 license (to match source header and snapd) * canonical/pebble#64 Avoid use of multipart.Part.FileName() to fix files API on Go 1.17 * canonical/pebble#63 Add wait-change API endpoint and client function * canonical/pebble#65 Make Pebble "go vet" clean * canonical/pebble#66 Remove a bunch of snapd references * canonical/pebble#67 Add API section to README; add HACKING.md; remove Makefile * canonical/pebble#60 Add Replan operation to restart changed services. * canonical/pebble#61 One-shot commands (pebble exec)
#13381 Includes these Pebble PRs: * canonical/pebble#59 * canonical/pebble#64 * canonical/pebble#63 * canonical/pebble#65 * canonical/pebble#66 * canonical/pebble#67 * canonical/pebble#60 * canonical/pebble#61
This adds an endpoint to the Pebble API to wait for a change to be ready, with an optional timeout. It's intended for use with the upcoming "exec" feature (spec and WIP PR). The LXD code has a similar feature, except changes are called "operations" in LXD, and there's a "wait operation" API endpoint.
This is particularly important for "pebble exec", so we can wait for the command to be finished without polling every short interval, and we can return a response immediately when the command finishes executing.
In future we could also use this new endpoint in the
pebble startandpebble stopcommands (and Python Operator Framework equivalents) to avoid polling there too.